dot/rpc implement state_subscribeStorage RPC WebSocket method by edwardmack · Pull Request #983 · ChainSafe/gossamer

edwardmack · 2020-07-06T19:19:58Z

Changes

Implement state_subscribeStorage RPC WebSocket method: Storage subscription. If storage keys are specified, it creates a message for each block which changes the specified storage keys. If none are specified, then it creates a message for every block. see https://github.com/w3f/PSPs/blob/psp-rpc-api/psp-002.md#state_subscribestorage-pubsub

Tests

go test ./dot/rpc/...

OR connect a websocket client to node and send request:

{
    "jsonrpc": "2.0",
    "method": "state_subscribeStorage",
    "params": [],
    "id": 1
}

Checklist

I have read CODE_OF_CONDUCT and CONTRIBUTING
I have provided as much information as possible and necessary
I have reviewed my own pull request before requesting a review
All integration tests and required coverage checks are passing

Issues

closes Implement State Subscribe Storage Websocket #969

edwardmack · 2020-07-06T19:31:09Z

I see that code climate is complaining, let me know if you want me to refactor.

noot · 2020-07-06T20:39:36Z

dot/rpc/modules/state.go

-	// TODO implement change storage trie so that block hash parameter works (See issue #834)
+// SubscribeStorage Storage subscription. If storage keys are specified, it creates a message for each block which
+//  changes the specified storage keys. If none are specified, then it creates a message for every block.
+//  This endpoint communicates over the Websockt protocol, but this func should remain here so it's added to rpc_methods list


noot · 2020-07-06T20:41:10Z

dot/rpc/modules/state.go

+// SubscribeStorage Storage subscription. If storage keys are specified, it creates a message for each block which
+//  changes the specified storage keys. If none are specified, then it creates a message for every block.
+//  This endpoint communicates over the Websockt protocol, but this func should remain here so it's added to rpc_methods list
+func (sm *StateModule) SubscribeStorage(r *http.Request, req *StateStorageQueryRangeRequest, res *StorageChangeSetResponse) error {


I'm also wondering, is there an unsubscribe method corresponding to the subscribe methods? if so, it would be nice to return the subscription ID

noot · 2020-07-06T20:42:16Z

dot/rpc/websocket.go

+		Filter:           filter,
 	}
 	wssub[sub] = wss
 	h.serverConfig.WSSubscriptions = wssub


the WSSubscriptions should be stored in the HTTPServer struct (or in a separate wsServer struct) not in the config

noot · 2020-07-06T20:42:36Z

dot/rpc/websocket.go

+		if change != nil {
+			for i, sub := range h.serverConfig.WSSubscriptions {
+				if sub.SubscriptionType == SUB_STORAGE {
+					// check it change key is in subscription filter


noot · 2020-07-06T20:43:26Z

dot/state/storage.go

+		Key:   key,
+		Value: value,
+	}
+	//fmt.Printf("SetStorage Key: %x Value: %x\n", key, value)


noot · 2020-07-06T20:43:34Z

dot/state/storage.go

+		Value: value,
+	}
+	//fmt.Printf("SetStorage Key: %x Value: %x\n", key, value)
+	// TODO ed add channel notify here


is this done?

should also be added to ClearStorage

noot

overall looking good, a few comments!

noot · 2020-07-06T20:48:10Z

dot/rpc/websocket.go

+	for change := range h.storageChan {
+
+		if change != nil {
+			for i, sub := range h.serverConfig.WSSubscriptions {


I'm wondering if it would be simpler to start a goroutine for each subscription, what do you think?

Yes, I've refactor websockets to better handle this.

edwardmack · 2020-07-13T21:05:59Z

I've updated this, it's ready for another look. Let me know your thoughts.

noot

great work! i have a few smaller comments but overall looks good :)

noot · 2020-07-14T15:35:50Z

dot/rpc/http.go

+type WSConn struct {
+	wsconn             *websocket.Conn
+	mu                 sync.Mutex
+	serverConfig       *HTTPServerConfig


does this need the whole server config? or just the state interfaces?

No just BlockAPI and StorageAPI, I've updated.

noot · 2020-07-14T15:36:02Z

dot/rpc/http.go

+	wsconn             *websocket.Conn
+	mu                 sync.Mutex
+	serverConfig       *HTTPServerConfig
+	logger             log.Logger


might be nice to make the logger global to the rpc package

noot · 2020-07-14T15:46:29Z

dot/rpc/websocket.go

+// Listen implementation of Listen interface to listen for channel changes
+func (l *StorageChangeListener) Listen() {
+	for change := range l.channel {
+		if change != nil {


nitpick but it might be nice to do

if change == nil { continue } // check if change key ....

same for below

edwardmack added 8 commits July 6, 2020 10:57

stub functions for storage change

8dcd114

create channels

1839679

added websocket subscription filter

e17115c

implement rpc state_subscribeStorage

cb68030

make lint

a4a06d1

add tests

4e92819

add tests

4dd45e3

update comments

452a7a1

edwardmack self-assigned this Jul 6, 2020

edwardmack requested a review from noot July 6, 2020 19:30

edwardmack added the Status: Needs Review label Jul 6, 2020

noot reviewed Jul 6, 2020

View reviewed changes

noot suggested changes Jul 6, 2020

View reviewed changes

noot reviewed Jul 6, 2020

View reviewed changes

edwardmack added 10 commits July 8, 2020 14:41

Merge branch 'development' into ed/subscribe_storage

d822106

move channel listener logic to individual go routines

631f942

update logging to use chainsafe logger

2696608

refactor websocket connection handling storage

b7ff1ce

refactor block listener

5bdbe0e

handle closing listening channels and websocket connections

9389b5b

lint issues

884c523

Merge branch 'development' into ed/subscribe_storage

0d62122

added notification on clear storage for storage change listener

ce4837c

handle closing channes and subscriptions

8d533ca

edwardmack added 6 commits July 11, 2020 16:53

implement close functions for block listener

46db2f6

fix test

0d19256

move websocket_test to service_test to fix cyclic import

005f4e6

fix lint issues

cb3c502

add tests for websocket

f9d65f0

update test

85b6602

noot self-requested a review July 14, 2020 15:34

noot approved these changes Jul 14, 2020

View reviewed changes

edwardmack added 5 commits July 15, 2020 09:31

Merge branch 'development' into ed/subscribe_storage

bd6e9d9

code cleanup

b0be8f1

add mock BlockAPI and mock StorageAPI to tests

cf9cc48

Merge branch 'development' into ed/subscribe_storage

31f6b5a

Merge branch 'development' into ed/subscribe_storage

bde2c68

edwardmack merged commit 5950064 into development Jul 16, 2020

edwardmack deleted the ed/subscribe_storage branch July 16, 2020 14:21

This was referenced Oct 19, 2022

Member Request edwardmack/seeding#1

Closed

Member Request polkadot-fellows/seeding#44

Merged

Conversation

edwardmack commented Jul 6, 2020

Changes

Tests

Checklist

Issues

Uh oh!

edwardmack commented Jul 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwardmack commented Jul 13, 2020

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noot Jul 6, 2020 •

edited

Loading